-
Notifications
You must be signed in to change notification settings - Fork 160
feat: allow caching the results of an unfiltered UniStream (WIP) #1853
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: allow caching the results of an unfiltered UniStream (WIP) #1853
Conversation
...a/ai/timefold/solver/core/impl/score/director/stream/BavetConstraintStreamScoreDirector.java
Outdated
Show resolved
Hide resolved
This is surprisingly simple! Some high-level comments; leaving naming out of the picture for now:
I'm not a big fan of this. The programming model doesn't look very nice IMO.
Something like this would be more stream-y:
This brings its own problems. But both proposals share the same issue - chaining static streams. Nothing (other than run-time fail-fasts) prevents you from doing This was the main idea of |
core/src/main/java/ai/timefold/solver/core/impl/bavet/common/tuple/RecordingTupleNode.java
Outdated
Show resolved
Hide resolved
|
||
@NullMarked | ||
public interface TupleSourceRoot<A> { | ||
void insert(A a); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure why this used to be @Nullable
, but I remember clearly that I was adding it there for a reason.
I recommend adding null checks, running the entire CI incl. quickstarts, and if the null checks don't trigger, then I say it's safe for this to be non-null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is for neighboorhoods; all the failing tests (when a null check is added to AbstractForEach) are in ai.timefold.solver.core.impl.neighborhood.*
. (ChangeMoveDefinition
, ListChangeMoveDefinitionTest
, UniEnumeratingStreamTest
and solveWithNeighborhoodsListVar
).
core/src/main/java/ai/timefold/solver/core/impl/bavet/uni/StaticDataUniNode.java
Outdated
Show resolved
Hide resolved
core/src/main/java/ai/timefold/solver/core/impl/bavet/uni/StaticDataUniNode.java
Outdated
Show resolved
Hide resolved
.../ai/timefold/solver/core/impl/score/stream/bavet/uni/BavetStaticDataUniConstraintStream.java
Outdated
Show resolved
Hide resolved
* As this is cached, it is vital the stream does not reference any variables | ||
* (genuine or otherwise). | ||
*/ | ||
<A> @NonNull UniConstraintStream<A> staticData(UniConstraintStream<A> stream); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens when this stream is used both statically and non-statically?
(Think node-sharing.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The output stream can be node shared if the input streams are identical (not implemented currently since this is a POC). The input stream will not be node shared; its an independent network.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The input stream may need to be shared though - why build the network twice if the user wants to reuse things?
To bring an alternative API idea:
This would produce a In essence, this brings a very simple, clear API:
It does have a downside - it doesn't allow arbitrary static streams, such as groupBy, map, flatten etc. Right now, nobody is asking for it, and I think it is a decent trade-off to get the benefits; but we can possibly ask other people for their opinion on this. |
ed2dfa9
to
0f4e6d0
Compare
0f4e6d0
to
6e94d9e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Submitting more comments as I get familiar with the changes.
core/src/main/java/ai/timefold/solver/core/api/score/stream/ConstraintFactory.java
Outdated
Show resolved
Hide resolved
core/src/main/java/ai/timefold/solver/core/api/score/stream/StaticDataFactory.java
Outdated
Show resolved
Hide resolved
core/src/main/java/ai/timefold/solver/core/api/score/stream/StaticDataSupplier.java
Outdated
Show resolved
Hide resolved
core/src/main/java/ai/timefold/solver/core/impl/bavet/common/AbstractStaticDataNode.java
Outdated
Show resolved
Hide resolved
core/src/main/java/ai/timefold/solver/core/impl/bavet/common/AbstractStaticDataNode.java
Outdated
Show resolved
Hide resolved
core/src/main/java/ai/timefold/solver/core/impl/bavet/quad/PrecomputeQuadNode.java
Show resolved
Hide resolved
core/src/main/java/ai/timefold/solver/core/impl/bavet/common/TupleSourceRoot.java
Outdated
Show resolved
Hide resolved
private final RecordingTupleLifecycle<Tuple_> recordingTupleLifecycle; | ||
private final Class<?>[] sourceClasses; | ||
|
||
public <Solution_> BavetStaticDataBuildHelper(BavetAbstractConstraintStream<Solution_> staticConstraintStream) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of this logic is very similar with the other build helper.
Consider sharing code.
core/src/main/java/ai/timefold/solver/core/impl/score/stream/common/RetrievalSemantics.java
Outdated
Show resolved
Hide resolved
core/src/main/java/ai/timefold/solver/core/api/score/stream/ConstraintFactory.java
Show resolved
Hide resolved
core/src/main/java/ai/timefold/solver/core/impl/bavet/common/TupleRecorder.java
Outdated
Show resolved
Hide resolved
} | ||
|
||
@Override | ||
public final void settle() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This exposes a confusing API.
Why is there a settle
method, and also a propagator
which allows external settling? Let's try to find a different solution; clear APIs have only 1 way of doing things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There a strong possibility of a performance regression happening if we introduce a new PropagationQueue type, since then the hot-loop of settle inside the NodeNetwork will have three valid interfaces of Propagator, and JVM method dispatch generate less efficient code when a call site can refer to three or move implementations of a class.
core/src/main/java/ai/timefold/solver/core/impl/bavet/NodeNetwork.java
Outdated
Show resolved
Hide resolved
core/src/main/java/ai/timefold/solver/core/impl/bavet/NodeNetwork.java
Outdated
Show resolved
Hide resolved
Good progress! What do you still need to do here? |
Implementation should be done; need more tests and a finalized design. |
- Introduces ConstraintFactory.staticData(Uni), which creates an independent NodeNetwork from the provided Uni stream. - Introduces TupleSourceRoot that represent top level tuple producers (the ForEach nodes now implement this, as well as a new StaticDataUniNode). - The StaticDataUniNode caches the results of its internal NodeNetwork and maps the tuples to new tuples compatiable with the external NodeNetwork - Inserts/retracts invalidate the cache and cause the output tuples to be updated. - Updates uses the cache and do not notify the internal NodeNetwork.
cf86173
to
d84061c
Compare
|
Introduces ConstraintFactory.staticData(Uni), which creates an independent NodeNetwork from the provided Uni stream.
Introduces TupleSourceRoot that represent top level tuple producers (the ForEach nodes now implement this, as well as a new StaticDataUniNode).
The StaticDataUniNode caches the results of its internal NodeNetwork and maps the tuples to new tuples compatiable with the external NodeNetwork
Inserts/retracts invalidate the cache and cause the output tuples to be updated.
Updates uses the cache and do not notify the internal NodeNetwork.